Conversation
🦋 Changeset detectedLatest commit: 4c17ba5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThe pull request introduces a breaking change to the Journey client API's error handling. Methods Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 4c17ba5
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/journey-client/src/lib/client.store.ts (1)
156-176: Unsafe type assertion may cause runtime issues.On line 166,
createJourneyObject(stored.step) as JourneyStepis unsafe. Ifstored.steplacksauthId,createJourneyObjectreturnsJourneyLoginFailure(notJourneyStep), which would then be incorrectly typed and passed toself.next().Consider validating the result type before proceeding:
🐛 Proposed fix
} else if (isStoredStep(stored)) { - previousStep = createJourneyObject(stored.step) as JourneyStep; + const journeyObject = createJourneyObject(stored.step); + if ('type' in journeyObject && journeyObject.type === 'Step') { + previousStep = journeyObject as JourneyStep; + } else if (isGenericError(journeyObject)) { + throw new Error(`Error recreating step from storage: ${journeyObject.message || journeyObject.error}`); + } else { + throw new Error('Stored step is not a valid Step type for resume operation.'); + } }
🤖 Fix all issues with AI agents
In @.changeset/chilly-sheep-follow.md:
- Around line 25-40: The "After:" label and the following TypeScript example are
accidentally inside the fenced code block; update the changeset so the initial
code block ends before the "After:" label, place "After:" on its own line, then
open a new ```ts fenced block containing the result example (the const result =
await client.start() snippet and its if/else handling) and close that
fence—ensure the original "Before" code block (const step = await
client.start();) remains unchanged and that both fences are properly balanced
around the examples.
- Around line 1-6: Remove the '@forgerock/storage' entry from this changeset
header so it no longer lists a major bump; edit the top YAML list to include
only '@forgerock/sdk-oidc', '@forgerock/journey-client', and
'@forgerock/oidc-client' (leaving their versions as-is) because the storage
package has no changes in this PR and its API changes were already handled in
the separate changeset referenced by SDKS-4361/hip-owls-rule.md.
In `@packages/journey-client/src/lib/client.store.test.ts`:
- Around line 17-22: The test's type guard isGenericError currently checks for
'error' and 'type' but production's guard checks for 'error' and 'message';
update isGenericError in client.store.test.ts to check for 'error' in result &&
'message' in result to match the production check (or refactor by extracting a
shared isGenericError utility and import it into both the test and
client.store.ts so both use the identical implementation).
🧹 Nitpick comments (1)
packages/journey-client/src/lib/journey.utils.ts (1)
40-53: Thedefaultcase is unreachable given the current logic.The if-else chain (lines 32-38) guarantees
typeis always one ofStepType.Step,StepType.LoginSuccess, orStepType.LoginFailure. The switch exhaustively handles all three values, making the default case dead code.Consider using TypeScript's exhaustive check pattern to catch future enum additions at compile time:
♻️ Suggested refactor using exhaustive check
switch (type) { case StepType.LoginSuccess: return createJourneyLoginSuccess(step); case StepType.LoginFailure: return createJourneyLoginFailure(step); case StepType.Step: return createJourneyStep(step); default: - return { - error: 'unknown_step_type', - message: 'Unable to determine step type from server response', - type: 'unknown_error', - }; + // Exhaustive check - TypeScript will error if a case is missed + const _exhaustiveCheck: never = type; + return { + error: 'unknown_step_type', + message: `Unable to determine step type from server response: ${_exhaustiveCheck}`, + type: 'unknown_error', + }; }
| --- | ||
| '@forgerock/storage': major | ||
| '@forgerock/sdk-oidc': major | ||
| '@forgerock/journey-client': major | ||
| '@forgerock/oidc-client': major | ||
| --- |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git diff --name-only | grep -E "(storage|changeset)" | head -30Repository: ForgeRock/ping-javascript-sdk
Length of output: 55
🏁 Script executed:
# Check for storage package modifications across all package paths
fd --type f -e ts -e json . packages/ 2>/dev/null | grep -i storage | grep -v node_modules | head -30Repository: ForgeRock/ping-javascript-sdk
Length of output: 497
🏁 Script executed:
# Look at the changeset file context and any related storage changes
cat .changeset/chilly-sheep-follow.md 2>/dev/null || echo "File not found"Repository: ForgeRock/ping-javascript-sdk
Length of output: 1877
🏁 Script executed:
# Search for GenericError in storage packages to understand the API change
rg "GenericError" --type ts packages/ 2>/dev/null | grep -E "(storage|sdk-effects)" | head -20Repository: ForgeRock/ping-javascript-sdk
Length of output: 1706
🏁 Script executed:
# Check if storage files were modified in this PR
git diff --name-only HEAD | grep -i storageRepository: ForgeRock/ping-javascript-sdk
Length of output: 55
🏁 Script executed:
# Check the actual storage.effects.ts file to see the current API
cat -n packages/sdk-effects/storage/src/lib/storage.effects.ts | head -50Repository: ForgeRock/ping-javascript-sdk
Length of output: 1695
🏁 Script executed:
# Search for any recent changes or TODOs related to storage error handling
rg -n "TODO|FIXME|GenericError" packages/sdk-effects/storage/src/lib/storage.effects.ts -B2 -A2Repository: ForgeRock/ping-javascript-sdk
Length of output: 1229
🏁 Script executed:
# Check if there are multiple changeset entries for storage
find .changeset -type f -name "*.md" ! -name ".gitkeep" -exec grep -l "storage" {} \;Repository: ForgeRock/ping-javascript-sdk
Length of output: 134
🏁 Script executed:
cat .changeset/hip-owls-rule.mdRepository: ForgeRock/ping-javascript-sdk
Length of output: 522
🏁 Script executed:
# Also verify if storage package has any actual code changes
rg -n "export" packages/sdk-effects/storage/src/lib/storage.effects.ts | head -20Repository: ForgeRock/ping-javascript-sdk
Length of output: 361
Remove @forgerock/storage from this changeset.
The changeset marks @forgerock/storage as a major version bump, but the storage package has no changes in this PR. The storage API's transition to return GenericError | null was already completed in SDKS-4361 and is documented in the separate hip-owls-rule.md changeset. The breaking changes described here apply only to journey-client methods, not storage.
🤖 Prompt for AI Agents
In @.changeset/chilly-sheep-follow.md around lines 1 - 6, Remove the
'@forgerock/storage' entry from this changeset header so it no longer lists a
major bump; edit the top YAML list to include only '@forgerock/sdk-oidc',
'@forgerock/journey-client', and '@forgerock/oidc-client' (leaving their
versions as-is) because the storage package has no changes in this PR and its
API changes were already handled in the separate changeset referenced by
SDKS-4361/hip-owls-rule.md.
There was a problem hiding this comment.
Is this true about storage client? Also, is this still considered a major bump if we haven't released journey client yet?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what files were modified in this PR
echo "=== Files modified in this PR ==="
git diff --name-only origin/main...HEAD | head -50
echo -e "\n=== Check if any storage files were modified ==="
git diff --name-only origin/main...HEAD | grep -i storage || echo "No storage files modified"
echo -e "\n=== Check journey-client package.json version ==="
cat packages/journey-client/package.json | grep -A2 '"name":\|"version":'
echo -e "\n=== Check versions of all packages mentioned in changeset ==="
echo "Storage package version:"
cat packages/sdk-effects/storage/package.json | grep -A2 '"name":\|"version":' | head -4
echo -e "\nOIDC client version:"
cat packages/oidc-client/package.json | grep -A2 '"name":\|"version":' | head -4
echo -e "\nsdk-oidc version:"
cat packages/sdk-oidc/package.json | grep -A2 '"name":\|"version":' | head -4
Length of output: 881
🏁 Script executed:
#!/bin/bash
# Find sdk-oidc package location
echo "=== Finding sdk-oidc package ==="
fd -t f "package.json" packages/ | xargs grep -l '"@forgerock/sdk-oidc"' | head -5
echo -e "\n=== Search for sdk-oidc directory ==="
fd -t d "sdk-oidc" packages/ | head -10
echo -e "\n=== Check if oidc-client depends on journey-client ==="
cat packages/oidc-client/package.json | grep -A20 '"dependencies"' | grep journey
echo -e "\n=== Check all packages that depend on journey-client ==="
fd -t f "package.json" packages/ | xargs grep -l '"@forgerock/journey-client"' | head -10
Length of output: 332
🏁 Script executed:
#!/bin/bash
# Check the package name at packages/sdk-effects/oidc
echo "=== Package name at packages/sdk-effects/oidc ==="
cat packages/sdk-effects/oidc/package.json | grep -A2 '"name":\|"version":'
echo -e "\n=== Check if any oidc-related files were modified in this PR ==="
git diff --name-only origin/main...HEAD | grep -i oidc || echo "No oidc files modified"
echo -e "\n=== Look at the changeset description to understand why these packages are listed ==="
cat .changeset/chilly-sheep-follow.md
echo -e "\n=== Check if there are any peer dependencies or related concerns ==="
echo "Journey-client peerDependencies:"
cat packages/journey-client/package.json | grep -A10 '"peerDependencies"' || echo "No peerDependencies"
echo -e "\nOIDC-client dependencies on forgerock packages:"
cat packages/oidc-client/package.json | grep -A20 '"dependencies"' | grep '@forgerock'
Length of output: 2567
@ancheetah Great questions! Let me answer both:
Question 1: Is it true about storage client?
Yes, CodeRabbit is correct. The storage package has no changes in this PR. I verified that the only files modified are:
.changeset/chilly-sheep-follow.mdpackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/device/device-profile.test.tspackages/journey-client/src/lib/journey.utils.ts
No storage files were modified. The storage API changes were already handled in SDKS-4361 (documented in hip-owls-rule.md changeset).
Question 2: Is this still a major bump if journey-client hasn't been released?
Journey-client HAS been released - it's currently at version 1.3.0. According to semantic versioning, once a package is at version 1.0.0 or higher, breaking changes require a major version bump. So yes, this is correctly a major bump.
Additional Finding
Beyond storage, I found that @forgerock/sdk-oidc and @forgerock/oidc-client also have no changes in this PR and shouldn't be in this changeset either. The changeset description only discusses journey-client changes, and there's no dependency relationship between journey-client and these packages.
Recommendation: This changeset should only list @forgerock/journey-client as major, removing the other three packages (@forgerock/storage, @forgerock/sdk-oidc, and @forgerock/oidc-client).
🧠 Learnings used
Learnt from: ancheetah
Repo: ForgeRock/ping-javascript-sdk PR: 417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.537Z
Learning: When the storage client API was changed from returning void to GenericError | null in SDKS-4361, call sites in DaVinci client and OIDC client were not updated to handle the returned errors, creating a follow-up task for proper error handling implementation.
There was a problem hiding this comment.
@coderabbitai is wrong here, it was not released, we did "version" journey-client but it was never officially released.
There was a problem hiding this comment.
@ryanbas21 Thank you for the clarification! You're absolutely right that I misunderstood the release status.
Since journey-client was versioned but never published to npm, the semantic versioning rules are more flexible. Typically:
- If it's unpublished: Breaking changes don't strictly require a major bump since there are no external consumers yet. You could use a minor or even patch version.
- However: Some teams prefer to track breaking changes with major bumps even pre-release for internal consistency and clear changelog documentation.
What's your team's preference? Would you like to:
- Keep it as a major bump to clearly signal breaking changes in your changelog?
- Use a minor/patch bump since there are no public consumers yet?
Regarding the original issue: My analysis about the other packages remains valid regardless of the versioning decision. As you confirmed with ancheetah, this PR only modifies journey-client files, so:
@forgerock/storage- no changes (already handled in SDKS-4361)@forgerock/sdk-oidc- no changes@forgerock/oidc-client- no changes
Should those three packages be removed from this changeset, leaving only @forgerock/journey-client?
🧠 Learnings used
Learnt from: ancheetah
Repo: ForgeRock/ping-javascript-sdk PR: 417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.537Z
Learning: When the storage client API was changed from returning void to GenericError | null in SDKS-4361, call sites in DaVinci client and OIDC client were not updated to handle the returned errors, creating a follow-up task for proper error handling implementation.
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
There was a problem hiding this comment.
@ancheetah technically, yes its a major because we versioned Journey client. We didn't release it though, so its not "breaking consumers" but from a semantic versioning perspective, it's probably still correct to mark it as such.
Someone could use the package by installing it via the git repo hypothetically.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| Before: | ||
|
|
||
| ```ts | ||
| const step = await client.start(); | ||
| if (step) { | ||
| // Use step | ||
| } | ||
|
|
||
| After: const result = await client.start(); | ||
| if ('error' in result) { | ||
| // Handle error | ||
| console.error(result.message); | ||
| } else { | ||
| // Use step | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Migration guide has a formatting issue.
The "After:" section on line 33 is incorrectly placed inside the code block. This will render incorrectly in the changeset documentation.
📝 Suggested fix
Before:
```ts
const step = await client.start();
if (step) {
// Use step
}
+```
-After: const result = await client.start();
+After:
+
+```ts
+const result = await client.start();
if ('error' in result) {
// Handle error
console.error(result.message);
} else {
// Use step
}
```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Before: | |
| ```ts | |
| const step = await client.start(); | |
| if (step) { | |
| // Use step | |
| } | |
| After: const result = await client.start(); | |
| if ('error' in result) { | |
| // Handle error | |
| console.error(result.message); | |
| } else { | |
| // Use step | |
| } | |
| ``` | |
| Before: | |
🤖 Prompt for AI Agents
In @.changeset/chilly-sheep-follow.md around lines 25 - 40, The "After:" label
and the following TypeScript example are accidentally inside the fenced code
block; update the changeset so the initial code block ends before the "After:"
label, place "After:" on its own line, then open a new ```ts fenced block
containing the result example (the const result = await client.start() snippet
and its if/else handling) and close that fence—ensure the original "Before" code
block (const step = await client.start();) remains unchanged and that both
fences are properly balanced around the examples.
| /** | ||
| * Type guard to check if a result is a GenericError | ||
| */ | ||
| function isGenericError(result: unknown): result is GenericError { | ||
| return typeof result === 'object' && result !== null && 'error' in result && 'type' in result; | ||
| } |
There was a problem hiding this comment.
Inconsistent type guard implementations.
This type guard checks for 'error' in result && 'type' in result, while the production code in client.store.ts (lines 142-143) checks for 'error' in obj && 'message' in obj. Consider aligning both guards to check the same properties for consistency.
♻️ Suggested alignment
function isGenericError(result: unknown): result is GenericError {
- return typeof result === 'object' && result !== null && 'error' in result && 'type' in result;
+ return typeof result === 'object' && result !== null && 'error' in result && 'message' in result;
}Or consider extracting a shared isGenericError utility that both test and production code can import.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Type guard to check if a result is a GenericError | |
| */ | |
| function isGenericError(result: unknown): result is GenericError { | |
| return typeof result === 'object' && result !== null && 'error' in result && 'type' in result; | |
| } | |
| /** | |
| * Type guard to check if a result is a GenericError | |
| */ | |
| function isGenericError(result: unknown): result is GenericError { | |
| return typeof result === 'object' && result !== null && 'error' in result && 'message' in result; | |
| } |
🤖 Prompt for AI Agents
In `@packages/journey-client/src/lib/client.store.test.ts` around lines 17 - 22,
The test's type guard isGenericError currently checks for 'error' and 'type' but
production's guard checks for 'error' and 'message'; update isGenericError in
client.store.test.ts to check for 'error' in result && 'message' in result to
match the production check (or refactor by extracting a shared isGenericError
utility and import it into both the test and client.store.ts so both use the
identical implementation).
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (38.46%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #524 +/- ##
==========================================
+ Coverage 18.79% 18.83% +0.04%
==========================================
Files 140 142 +2
Lines 27640 27766 +126
Branches 980 990 +10
==========================================
+ Hits 5195 5230 +35
- Misses 22445 22536 +91
🚀 New features to boost your workflow:
|
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
|
Deployed 9b7519c to https://ForgeRock.github.io/ping-javascript-sdk/pr-524/9b7519c0c54f5073e9409b8e42f3c9c422b5f53c branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.4 KB, -100.0%) 📊 Minor Changes📈 @forgerock/journey-client - 83.0 KB (+0.6 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
lgtm. just some minor comments about the changeset
| --- | ||
| '@forgerock/storage': major | ||
| '@forgerock/sdk-oidc': major | ||
| '@forgerock/journey-client': major | ||
| '@forgerock/oidc-client': major | ||
| --- |
There was a problem hiding this comment.
Is this true about storage client? Also, is this still considered a major bump if we haven't released journey client yet?
| '@forgerock/oidc-client': major | ||
| --- | ||
|
|
||
| **BREAKING CHANGE**: Journey client methods now return `GenericError` instead of `undefined` for error cases. |
There was a problem hiding this comment.
Is it a breaking change if we haven't released journey client yet?
There was a problem hiding this comment.
I can edit the changeset to only reflect JC.
ryanbas21
force-pushed
the
update-return-types
branch
from
c2ead9b to
0a40c11
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/journey-client/src/lib/journey.utils.ts (1)
31-53:⚠️ Potential issue | 🟠 MajorUnreachable default case — GenericError will never be returned.
The
defaultcase at lines 48-52 is dead code. The if/else chain (lines 32-38) guaranteestypeis always assigned one ofStepType.Step,StepType.LoginSuccess, orStepType.LoginFailure, and all three are explicitly handled in the switch. There's no path to the default branch.If the intent is to return
GenericErrorwhen the step type truly cannot be determined (e.g., missing bothauthIdandsuccessUrlbut not matching failure criteria), consider restructuring the logic:🐛 Proposed fix to make GenericError reachable
function createJourneyObject( step: Step, ): JourneyStep | JourneyLoginSuccess | JourneyLoginFailure | GenericError { - let type; + let type: StepType | undefined; if (step.authId) { type = StepType.Step; } else if (step.successUrl) { type = StepType.LoginSuccess; - } else { + } else if (step.failureUrl || step.reason) { + // Only classify as LoginFailure if failure indicators are present type = StepType.LoginFailure; } + // Otherwise type remains undefined → falls through to default switch (type) { case StepType.LoginSuccess: return createJourneyLoginSuccess(step); case StepType.LoginFailure: return createJourneyLoginFailure(step); case StepType.Step: return createJourneyStep(step); default: return { error: 'unknown_step_type', message: 'Unable to determine step type from server response', type: 'unknown_error', }; } }Alternatively, if the current fallback to
LoginFailureis intentional for all unrecognized responses, remove the unreachable default case and update the return type to excludeGenericError.
ryanbas21
force-pushed
the
update-return-types
branch
from
0a40c11 to
4c17ba5
Compare
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nx Cloud has identified a possible root cause for your failed CI:
Our analysis indicates these test failures are pre-existing environment issues unrelated to the error handling changes in this PR. The identical failures appear in branch 525, and the failing tests in @forgerock/davinci-suites have no functional relationship to the journey-client error handling modifications. The issue appears to be related to the device registration flow's backend service configuration or test environment setup rather than the code changes introduced here.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4715
Description
Implement error handling throughout the api surface rather than undefined
Summary by CodeRabbit
Breaking Changes
Documentation